Skip to content

Conversation

@eriknw
Copy link
Member

@eriknw eriknw commented Dec 7, 2016

Should fix dask/distributed#725

This was an oversight that should have been handled in #326. We only tested objects defined in toolz.functoolz, so pickling accidentally worked, because they were in the same module as curry.

…ried objects.

Should fix dask/distributed#725

This was an oversight that should have been handled in pytoolz#326.  We only tested
objects defined in `toolz.functoolz`, so pickling accidentally worked, because
they were in the same module as `curry`.
@llllllllll
Copy link
Contributor

Thanks for fixing this

@eriknw
Copy link
Member Author

eriknw commented Dec 7, 2016

Sure thing, my bad for the oversight. Time for a bug-fix release?


self.__doc__ = getattr(func, '__doc__', None)
self.__name__ = getattr(func, '__name__', '<curry>')
self.__module__ = getattr(func, '__module__', None)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not use functools.wraps or functools.update_wrapper? See https://docs.python.org/2.7/library/functools.html#functools.update_wrapper

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been bitten by those functions before, but I suppose we were bitten by not using them since __module__ wasn't being set but should have.

Mostly, I want control over how curry behaves, and being explicit makes it easier for us to implement and test cytoolz.curry.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you probably want to copy __qualname__ on Python 3 as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want #335. This was also explored and discussed in #230. My bad for letting things linger.

Things can get convoluted, though, so I prefer straightforward and explicit even if it's a little more verbose. For example, __annotations__ will need to be custom-handled for curry much like curry.__signature__.

To your point, though, I'll go ahead and add __qualname__ to curry in Python 3.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If available, is __qualname__ used during pickling? If so, I'd like to test this behavior in this PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIR, it is. It should allow pickling class methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the critical eye and expert knowledge, @pitrou! More tests coming...

This isn't pretty, but hopefully tests provide adequate coverage.

This includes a signficant change to `curry`: we defined `__getattr__` to
get attributes from the wrapped func.
@eriknw
Copy link
Member Author

eriknw commented Dec 8, 2016

Okay, we now take advantage of __qualname__ to better support serialization of nested objects. This is not pretty, and apparently Python 3.3 and 3.4 fail hard when using __qualname__ during serialization. Go figure. I look forward to the day when I only need to be concerned with Python 3.5 and above.

For more robust serialization support, curry now defines __getattr__, which gets attributes from self.func if not found on the curried object. Thoughts?

@pitrou
Copy link

pitrou commented Dec 8, 2016

For more robust serialization support, curry now defines getattr, which gets attributes from self.func if not found on the curried object. Thoughts?

My intuition would be to only copy a well-known set of attributes rather than forward all of them.

@eriknw
Copy link
Member Author

eriknw commented Dec 8, 2016

At a minimum, we can choose not to forward dunder attributes. Forwarding other attributes allows non-curried attributes with __qualname__ of the wrapped object to be pickled normally. Without this, many of the test_curried_qualname tests fail.

Learning how to be a good Python citizen w.r.t. __qualname__ when decorating objects is new for me, and I suspect it's new for most people. I don't know of best practices here. I care about supporting serialization, though, so it seems reasonable to me to do the best we can.

curry is by far the most sophisticated object in toolz. It has a lot of custom, but reasonable, behaviors. I like that it's distinct from the wrapped object, because curry is so different from other objects. We have not tried to make curry look like the wrapped object. Forwarding attributes via __getattr__ makes curry look more like the wrapped object, which I'm -0.5 on. I don't think it's terrible behavior, though, and I suspect some people will prefer it. I'm +1 on more robust serialization.

One final comment regarding functools.wraps and functools.update_wrapper. I've had to remove dunders specific to Python 3 from objects in toolz, because they resulted in incorrect behavior. This is why I'm -1 on functools.wraps. Dunders imply behavior, so I don't want to copy or set them unless that behavior is understood, desired, and tested.

@eriknw
Copy link
Member Author

eriknw commented Dec 8, 2016

I think there may be a Python enhancement to learn from this experience: when pickling objects with __qualname__, try following __wrapped__ attributes to find the desired object. This should allow __qualname__-enabled pickling to work when objects in the qualified path have been decorated.

…affects.

IMHO, this is a failure of how Python handles `__qualname__` during pickling.
Objects wrapped by decorators should only need to use `__wrapped__` for
pickling to work as expected.  It's unreasonable to expect wrapping objects
such as `curry` to forward attributes to allow pickling to work.
@eriknw
Copy link
Member Author

eriknw commented Dec 9, 2016

I removed curry.__getattr__, because I think it's unreasonable for us to have to do that. I commented out the tests this affects. If anybody requests this, it will be easy to add back in and test.

I'll look into enhancing Python to better support pickling decorated objects. Unfortunately, __wrapped__ has other implications, and we are unable to set self.__wrapped__ to self.func, so the best way to proceed is not clear to me.

@pitrou
Copy link

pitrou commented Dec 9, 2016

I'll look into enhancing Python to better support pickling decorated objects.

The common way is to use functools.wraps (or functools.update_wrapper) to propagate the appropriate metadata.

@eriknw
Copy link
Member Author

eriknw commented Dec 9, 2016

@pitrou, I know that. They don't help here, although I would be delighted if you could correct me.

Here's an example of what #326 and this PR are meant to resolve (ignoring __qualname__ for now):

Python 3.5.2 |Continuum Analytics, Inc.| (default, Jul  2 2016, 17:52:12) 
[GCC 4.2.1 Compatible Apple LLVM 4.2 (clang-425.0.28)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import functools
>>> import pickle
>>> 
>>> class decorator:
...     def __init__(self, func):
...         self.__wrapped__ = func
...         functools.update_wrapper(self, func)
...     def __call__(self, *args, **kwargs):
...         print('calling decorated function')
...         return self.__wrapped__(*args, **kwargs)
... 
>>> @decorator
... def f(x):
...     return x 
... 
>>> f(1)
calling decorated function
1
>>> pickle.dumps(f, -1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
_pickle.PicklingError: Can't pickle <function f at 0x101b79c80>: it's not the same object as __main__.f

It's common--and convenient--for curry to be used as a decorator in the main namespace of a module. We need to be able to pickle the curried object in the top-level namespace and derived objects that have curried arguments. It would be nice if we could pickle other things too, like nested objects, the original wrapped object, instances of the original object, methods on the original object, etc. It would be best (IMHO) if pickle itself handled much of this work for us.

We support what we must support, as shown below, but it would be great if serialization were more robust.

Python 3.5.2 |Continuum Analytics, Inc.| (default, Jul  2 2016, 17:52:12) 
[GCC 4.2.1 Compatible Apple LLVM 4.2 (clang-425.0.28)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import pickle
>>> from toolz import curry
>>> 
>>> @curry
... def f(x, y):
...     return x + y
... 
>>> pickle.loads(pickle.dumps(f)) is f
True
>>> 
>>> f1 = f(1)
>>> f1_new = pickle.loads(pickle.dumps(f1))
>>> f1(2) == f1_new(2) == 3
True

Pickling that leverages __qualname__ is very fragile when an object in the qualified path has been decorated. This is what curry.__getattr__ was used to circumvent.

@eriknw eriknw merged commit be3ce86 into pytoolz:master Dec 10, 2016
eriknw added a commit to eriknw/cytoolz that referenced this pull request Dec 11, 2016
Also, fix signature registry for flip and memoize.  I had forgotten
that we can list multiple signatures.
eriknw added a commit to pytoolz/cytoolz that referenced this pull request Dec 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't pickle function delayed

3 participants